@claude review

Claude finished @cliffhall's task in 1m 13s —— View job

Code Review

• Read the changed file and diff
• Analyze the fix correctness
• Check for related issues or improvements
• Post review feedback

Verdict: LGTM — correct, minimal fix.

What changed

-      await expect(
-        manager.addObservations([
+      await expect(async () => {
+        await manager.addObservations([
           { entityName: 'NonExistent', contents: ['some observation'] },
-        ])
-      ).rejects.toThrow('Entity with name NonExistent not found');
+        ]);
+      }).rejects.toThrow('Entity with name NonExistent not found');

src/memory/__tests__/knowledge-graph.test.ts, line 149–153

Root cause analysis

Looking at the implementation in src/memory/index.ts (lines 139–152), addObservations is declared async but the actual throw happens synchronously inside the .map() callback:

async addObservations(...): Promise<...> {
  const graph = await this.loadGraph();
  const results = observations.map(o => {
    const entity = graph.entities.find(e => e.name === o.entityName);
    if (!entity) {
      throw new Error(`Entity with name ${o.entityName} not found`); // sync throw
    }
    ...
  });
  ...
}

A synchronous throw inside an async function body is caught by the async machinery and converted to a rejected Promise — but it resolves on the very next microtask tick. When the original code passed the resulting Promise directly to expect(promise).rejects, there was a race: Vitest's global unhandled-rejection handler could capture the rejection before .rejects had a chance to attach its own handler.

Wrapping in async () => { await ... } hands Vitest a function rather than an already-rejected Promise. Vitest calls the function and attaches its rejection interceptor first, so the race no longer exists. This is the recommended pattern in the Vitest docs.

Review notes

• Correctness: The fix is correct and eliminates the race condition as described.
• Scope: Only the one affected test is changed; no other tests in the file pass Promises directly to .rejects, so no other changes are needed.
• Minimality: 4 lines in, 4 lines out — no unnecessary churn.
• PR description accuracy: The description correctly identifies the root cause (sync throw inside async function passed directly as a Promise).
• No production code changes: The fix is entirely in the test; the addObservations implementation is untouched (appropriate, since the implementation behaviour is correct).

This is a clean, well-understood fix for a known Vitest/Jest async-rejection timing issue. Already approved by @olaservo. Ready to merge.